Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Enabling DefaultAzureCredential for AuthenticationMode #2578

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat: Enabling DefaultAzureCredential for AuthenticationMode #2578

wants to merge 8 commits into from
+68 −1

Conversation

jayendranarumugam
Copy link

@jayendranarumugam jayendranarumugam commented Nov 24, 2024 •
edited
Loading

Enabling DefaultAzureCredential for AuthenticationMode can support local debug/development operations and also can work on several Azure resources as well

With this improvement, we can now able to hit the default switch statement which earlier not possible

            switch (authenticationInfo.Mode)
            {
                case AuthenticationMode.ServicePrincipal:
                    tokenCredential = new ClientSecretCredential(tenantId, authenticationInfo.IdentityId, authenticationInfo.Secret, tokenCredentialOptions);
                    break;
                case AuthenticationMode.UserAssignedManagedIdentity:
                    var clientId = authenticationInfo.GetIdentityIdOrDefault();
                    tokenCredential = new ManagedIdentityCredential(clientId, tokenCredentialOptions);
                    break;
                case AuthenticationMode.SystemAssignedManagedIdentity:
                    tokenCredential = new ManagedIdentityCredential(options:tokenCredentialOptions);
                    break;
                default:
                    tokenCredential = new DefaultAzureCredential();   // <-- this can possible now
                    break;
            }

I successfully tested this from my local and validated the Unit Test cases. Please let me know your thoughts on this feature

@CLAassistant
Copy link

CLAassistant commented Nov 24, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions GitHub Actions
Copy link

Thank you for your contribution! 🙏 We will review it as soon as possible.

@jayendranarumugam jayendranarumugam changed the title Enabling DefaultAzureCredential for AuthenticationMode feat: Enabling DefaultAzureCredential for AuthenticationMode Nov 24, 2024
@jayendranarumugam jayendranarumugam marked this pull request as ready for review November 24, 2024 10:32
tomkerkhove
tomkerkhove requested changes Nov 25, 2024
View reviewed changes
Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the configuration, 1 suggestion.

Can you also please open a PR for the docs?

changelog/content/deprecated/authentication-modes.md Outdated Show resolved Hide resolved
@jayendranarumugam jayendranarumugam mentioned this pull request Nov 25, 2024
Merged
@jayendranarumugam
Copy link
Author

Opened a PR https://github.com/promitor/docs/pull/67/files for relevant docs update

@jayendranarumugam jayendranarumugam mentioned this pull request Nov 25, 2024
Open
@tomkerkhove
Copy link
Owner

@jayendranarumugam Mind fixing the code quality?

@jayendranarumugam
Copy link
Author

@tomkerkhove, sure, this is relatively new to me. Please correct my understanding; we have to fix all the

##[notice]"[]...... to make this code quality to pass ?

@tomkerkhove
Copy link
Owner

It's fine if you just focus on the warnings, don't worry! (Warning: "[)

@jayendranarumugam
Copy link
Author

@tomkerkhove hope now all resolved . Thanks for the help ! Can you take a look and merge this ?

@tomkerkhove
Copy link
Owner

@jayendranarumugam Looks like we are not there yet, mind fixing build issue please?

@jayendranarumugam
Copy link
Author

jayendranarumugam commented Dec 2, 2024 •
edited
Loading

@tomkerkhove pushed updates, could you trigger the code scan and CI workflow now ?

@jayendranarumugam
Copy link
Author

@tomkerkhove checked the latest failed logs on code quality which seems confusing me

Earlier the code quality failed with

##[warning]"[InconsistentNaming] Name 'azureMetricConfigurationBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'AzureMetricConfigurationBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(22,60)

So after i renamed the variable as per the suggestion i.e AzureMetricConfigurationBase

But now i got the error to undo it the old way

##[warning]"[InconsistentNaming] Name 'AzureMetricConfigurationBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'azureMetricConfigurationBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(22,60)

@tomkerkhove
Copy link
Owner

I believe these are the ones to tackle, no?

Warning: "[InconsistentNaming] Name 'AzureMetricConfigurationBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'azureMetricConfigurationBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(22,60)
Warning: "[InconsistentNaming] Name 'AzureMetricConfigurationBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'azureMetricConfigurationBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(24,60)
Warning: "[InconsistentNaming] Name 'AzureMetricNameBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'azureMetricNameBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(18,40)
Warning: "[InconsistentNaming] Name 'AzureMetricNameBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'azureMetricNameBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(20,40)
Warning: "[InconsistentNaming] Name 'AzureMonitorMeter' does not match rule 'Static readonly fields (private)'. Suggested name is 'azureMonitorMeter'." on /home/runner/work/promitor/promitor/src/Promitor.Integrations.Sinks.OpenTelemetry/OpenTelemetryCollectorMetricSink.cs(20,39)
Warning: "[InconsistentNaming] Name 'BatchSize' does not match rule 'Static readonly fields (private)'. Suggested name is 'batchSize'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(45,37)
Warning: "[InconsistentNaming] Name 'Bogus' does not match rule 'Static readonly fields (private)'. Suggested name is 'bogus'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Generators/ScrapeResultGenerator.cs(10,39)
Warning: "[InconsistentNaming] Name 'DefaultAggregationInterval' does not match rule 'Static readonly fields (private)'. Suggested name is 'defaultAggregationInterval'." on /home/runner/work/promitor/promitor/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/AggregationDeserializer.cs(9,42)
Warning: "[InconsistentNaming] Name 'DefaultInterval' does not match rule 'Static readonly fields (private)'. Suggested name is 'defaultInterval'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Serialization/DeserializerTests/DeserializationTests.cs(13,42)
Warning: "[InconsistentNaming] Name 'EmptyResourceDefinitions' does not match rule 'Static readonly fields (private)'. Suggested name is 'emptyResourceDefinitions'." on /home/runner/work/promitor/promitor/src/Promitor.Agents.Scraper/Discovery/StubResourceDiscoveryRepository.cs(11,63)
Warning: "[InconsistentNaming] Name 'HealthReport' does not match rule 'Static readonly fields (private)'. Suggested name is 'healthReport'." on /home/runner/work/promitor/promitor/src/Promitor.Agents.Scraper/Discovery/StubResourceDiscoveryRepository.cs(12,51)
Warning: "[InconsistentNaming] Name 'LogAnalyticsConfigurationBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'logAnalyticsConfigurationBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(30,61)
Warning: "[InconsistentNaming] Name 'LogAnalyticsConfigurationBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'logAnalyticsConfigurationBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(32,61)
Warning: "[InconsistentNaming] Name 'NamingConvention' does not match rule 'Static readonly fields (private)'. Suggested name is 'namingConvention'." on /home/runner/work/promitor/promitor/src/Promitor.Core/Serialization/Yaml/YamlSerialization.cs(8,51)
Warning: "[InconsistentNaming] Name 'PrometheusMetricDefinition' does not match rule 'Static readonly fields (private)'. Suggested name is 'prometheusMetricDefinition'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(19,60)
Warning: "[InconsistentNaming] Name 'PrometheusMetricDefinitionTest' does not match rule 'Static readonly fields (private)'. Suggested name is 'prometheusMetricDefinitionTest'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(21,60)
Warning: "[InconsistentNaming] Name 'ResourceGroupNameTest' does not match rule 'Static readonly fields (private)'. Suggested name is 'resourceGroupNameTest'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(44,40)
Warning: "[InconsistentNaming] Name 'ScrapingBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'scrapingBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(38,44)
Warning: "[InconsistentNaming] Name 'ScrapingBase' does not match rule 'Static readonly fields (private)'. Suggested name is 'scrapingBase'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(40,44)
Warning: "[InconsistentNaming] Name 'SubscriptionId' does not match rule 'Static readonly fields (private)'. Suggested name is 'subscriptionId'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Metrics/ScrapeDefinitionBatchPropertiesTest.cs(21,40)
Warning: "[InconsistentNaming] Name 'SubscriptionIdTest' does not match rule 'Static readonly fields (private)'. Suggested name is 'subscriptionIdTest'." on /home/runner/work/promitor/promitor/src/Promitor.Tests.Unit/Core/Scraping/Batching/AzureResourceDefinitionBatchingTests.cs(23,40)

@jayendranarumugam
Copy link
Author

Yep, for these only i made some fix earlier but now the error is showing to undo the fix that i made and suggest to revert it back to normal

Check the old error -> https://github.com/tomkerkhove/promitor/actions/runs/12007318471/job/33468659849
After i made the suggestion as per the old error above
New error - > https://github.com/tomkerkhove/promitor/actions/runs/12117711249/job/33809090751
which suggest to make it the old way itself

@jayendranarumugam
Copy link
Author

@tomkerkhove could you pls review this now ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkerkhove tomkerkhove Awaiting requested review from tomkerkhove tomkerkhove is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jayendranarumugam @CLAassistant @tomkerkhove